Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

stake: Advance credits_observed on activation epoch#19309

Merged
joncinque merged 2 commits intosolana-labs:masterfrom
joncinque:st-act-adv
Sep 3, 2021
Merged

stake: Advance credits_observed on activation epoch#19309
joncinque merged 2 commits intosolana-labs:masterfrom
joncinque:st-act-adv

Conversation

@joncinque
Copy link
Copy Markdown
Contributor

@joncinque joncinque commented Aug 19, 2021

Problem

A stake does not receive rewards during its activation epoch, since stake.delegation.stake(... always returns 0 for the activation epoch [1]. On the other hand its credits_observed is not updated, which can make reward calculation confusing.

[1]

} else if target_epoch == self.activation_epoch {
// all is activating
(0, delegated_stake)

Summary of Changes

Advance credits_observed during the activation epoch. My approach was to reuse the rewarded_epoch being used elsewhere rather than going off the vote_state.epoch_credits() as used in calculate_stake_points_and_credits. It means there's some redundancy, but it felt clearer to me.

Edit: this was built on #19308, and rebased since then to isolate the changes

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 20, 2021

Codecov Report

Merging #19309 (695e6b4) into master (c550b32) will decrease coverage by 0.1%.
The diff coverage is 43.2%.

@@            Coverage Diff            @@
##           master   #19309     +/-   ##
=========================================
- Coverage    82.7%    82.5%   -0.2%     
=========================================
  Files         461      468      +7     
  Lines      131215   131703    +488     
=========================================
+ Hits       108519   108733    +214     
- Misses      22696    22970    +274     

Copy link
Copy Markdown
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

t-nelson
t-nelson previously approved these changes Aug 21, 2021
@stale
Copy link
Copy Markdown

stale Bot commented Aug 28, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 28, 2021
@joncinque joncinque removed the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 30, 2021
@mergify mergify Bot dismissed t-nelson’s stale review September 3, 2021 11:59

Pull request has been modified.

@joncinque joncinque merged commit 2c3bded into solana-labs:master Sep 3, 2021
@joncinque joncinque deleted the st-act-adv branch September 3, 2021 21:20
mergify Bot pushed a commit that referenced this pull request Sep 3, 2021
* stake: Advance `credits_observed` on activation epoch

* Add test for merging stakes just after activation

(cherry picked from commit 2c3bded)

# Conflicts:
#	runtime/src/bank.rs
mergify Bot added a commit that referenced this pull request Sep 4, 2021
… (#19626)

* stake: Advance `credits_observed` on activation epoch (#19309)

* stake: Advance `credits_observed` on activation epoch

* Add test for merging stakes just after activation

(cherry picked from commit 2c3bded)

# Conflicts:
#	runtime/src/bank.rs

* Fix merge issues

Co-authored-by: Jon Cinque <jon.cinque@gmail.com>
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants